7124: Add focus and maximizedFocus launch modes by Don-Vito · Pull Request #7873 · microsoft/terminal

Don-Vito · 2020-10-09T14:16:53Z

This commit introduces two new launch modes: focus and maximizedFocus.

Focused mode, behaves like a default mode, but with the Focus Mode
enabled.
Maximized focused mode, behaves like a Maximized mode, but with the
Focus Mode enabled.

There two ways to invoke these new modes:

In the settings file: you set the "launchMode" to either "focus" or
"maximizedFocus"
In the command line options, you can path -f / --focus, which is
mutually exclusive with the --fullscreen, but can be combined with the
--maximized:
- Passing -f / --focus will launch the terminal in the "focus" mode
- Passing -fM / --focus --maximized will launch the terminal in the
  "maximizedFocus" mode

This should resolve a relevant part in the command line arguments
mega-thread #4632

Closes #7124
Closes #7825
Closes #7875

… If one of them is set, focusedMode should be toggled on startup

Mike Griese (zadjii-msft) · 2020-10-09T14:38:49Z

I love this. I think you've done the focused mode thing exactly right. My one concern - I'd yank the maximizedFocus mode. In my opinion, that's just the same thing as fullscreen mode 😄

Don-Vito · 2020-10-09T14:53:13Z

I love this. I think you've done the focused mode thing exactly right. My one concern - I'd yank the maximizedFocus mode. In my opinion, that's just the same thing as fullscreen mode 😄

The only difference from the fullscreen is that in the maximized focused mode, you still see the taskbar (not sure how useful it is). Added this mode as it seems that in was originally mentioned in the feature request (#7124): "I want to have maximized and focused mode when app is launch" (#7124 (comment))

Please let me know if you prefer me to delete anyway.

Mike Griese (zadjii-msft) · 2020-10-09T15:09:32Z

Oh, fair point! Then I'm totally cool with this. I don't hate maximizedFocus either - I think maximized|focus would be wrong, because that would imply the whole set is flags that could be combined (they aren't). It's basically either maximizedFocus or focusMaximized, and I don't feel like there's a tangible difference between the two, so let's go with what's already been implemented 😛

Don-Vito · 2020-10-09T15:21:54Z

Oh, fair point! Then I'm totally cool with this. I don't hate maximizedFocus either - I think maximized|focus would be wrong, because that would imply the whole set is flags that could be combined (they aren't). It's basically either maximizedFocus or focusMaximized, and I don't feel like there's a tangible difference between the two, so let's go with what's already been implemented 😛

Yay :)
Should I add some UTs? If so - can you please guide me to the framework that I can base on?
If not - should I mark this as "Ready for review"? (I am quite new with the Draft PRs, work with Azure DevOps mostly).

Mike Griese (zadjii-msft) · 2020-10-09T15:29:34Z

Should I add some UTs?

Bless your soul ☺️

In this specific case, since this is such a UI-dependent feature, I think we're cool not adding any unit tests. The enum parsing is already pretty well tested, and I'm not sure we've got any sort of framework for testing the state of the Win32 window. I'd definitely mark this as "Ready for Review" 😄

Don-Vito · 2020-10-10T08:57:58Z

Mike Griese (@zadjii-msft) - sorry for pushing another change after the approval (I was sure that this new commit will reset it, but for some reason it didn't).

I have noticed that in the megathread (#4632) there is also a request to add command-line option for this. So added it as well.

Mike Griese (zadjii-msft)

are you a mind reader?

I was gonna go ahead and sneak the cmdline argument in after this PR merged, but hey, you've gone and already done it. Thanks for the diligence! I quite like the elegance of -Mf for combining focus and maximized too

Dustin L. Howett (DHowett)

I believe we'll need to update the schema in doc/cascadia! This looks excellent otherwise.

Don-Vito · 2020-10-13T06:43:36Z

Added the schema to the cascadia docs. Should I do anything beyond validating that it works on different profile?
This schema corresponds to the documentation PR I put in the docs repo (MicrosoftDocs/terminal#158)

Leonard Hecker (lhecker)

I believe you can drop MaximizedFocusMode.
You could get the same effect by calling wt.exe --focus --maximized instead, couldn't you?

Leonard Hecker (lhecker) · 2020-10-13T10:17:55Z

    //   -F,--fullscreen: Fullscreens the window on launch
    auto maximizedCallback = [this](int64_t /*count*/) {
-        _launchMode = winrt::Microsoft::Terminal::Settings::Model::LaunchMode::MaximizedMode;
+        _launchMode = (_launchMode.has_value() && _launchMode.value() == winrt::Microsoft::Terminal::Settings::Model::LaunchMode::FocusMode) ?


You can drop the winrt::Microsoft::Terminal::Settings::Model:: prefix, because the file already imports the namespace. 👍

Thanks! Done!

Don-Vito · 2020-10-13T20:14:28Z

I believe you can drop MaximizedFocusMode.
You could get the same effect by calling wt.exe --focus --maximized instead, couldn't you?

Leonard Hecker (@lhecker) - please guide me here. How do you suggest to encode the combination of --focus --maximized instead?

…ndLineArgs

Don-Vito · 2020-10-14T06:22:42Z

Dustin L. Howett (@DHowett) - sorry for nagging, but CI is somewhat harsh with me today and I am not sure if I can somehow unblock this PR 😄

The github spelling complains about something I already fixed (I wrote betwen instead of between).
The OutputTests::BasicReadConsoleOutputATest fails - though it passes locally for me, and I can hardly follow the link between it and the change.

Dustin L. Howett (DHowett)

Excellent, excellent stuff. Thank you!

Dustin L. Howett (DHowett) · 2020-10-14T22:19:02Z

I have several questions regarding the feature and the PR

This is my first PR, please be gentle

This is excellent work for a first PR -- though your other work merged first! 😄
We're happy to have you.

I hesitated between introducing new launch modes or adding a separate setting for "startInFocused" mode. I >preferred not to introduce a new parameter, was this a good decision?

Looks like we went w/ a parameter.

Didn't updated the documentation yet - wanted to hear your opinion first.

All set!

Not sure how to test this in UT. Didn't find UT for other modes.

Mike Griese (@zadjii-msft) summed it up quite nicely.

Didn't add those modes to args. Please let me know if required (and if so, the names of the options)

All set again! 😁

Deleted user (ghost) · 2020-10-14T22:19:30Z

Hello Dustin L. Howett (@DHowett)!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

## Summary of the Pull Request Since we've started working on the Settings UI, a few settings have been added on `main`. This adds those missing settings over. ## References Missing settings include... - #7364: `disableAnimations` - #7873: `launchMode` `focus` and `maximizedFocus` - #7793: `bellStyle` ## Validation Steps Performed Verified that those settings appear properly in the Settings UI.

Deleted user (ghost) · 2020-11-11T18:14:17Z

🎉Windows Terminal Preview v1.5.3142.0 has been released which incorporates this pull request.:tada:

Handy links:

7124: initial commit - add focused and maximizedFocused launch modes.…

f5b5e33

… If one of them is set, focusedMode should be toggled on startup

ghost added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Oct 9, 2020

7124: fix code formatting

a2d3662

Mike Griese (zadjii-msft) self-assigned this Oct 9, 2020

Mike Griese (zadjii-msft) added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 9, 2020

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 9, 2020

Mike Griese (zadjii-msft) reviewed Oct 9, 2020

View reviewed changes

Comment thread src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h Resolved

7124: rename focused -> focus, both in the settings name and in the code

bb0d3b1

Mike Griese (zadjii-msft) approved these changes Oct 9, 2020

View reviewed changes

Mike Griese (zadjii-msft) removed their assignment Oct 9, 2020

Don-Vito marked this pull request as ready for review October 9, 2020 16:04

Don-Vito changed the title ~~7124: Add focused and maximizedFocused launch modes~~ 7124: Add focus and maximizedFocus launch modes Oct 9, 2020

This was referenced Oct 9, 2020

7124: add documentation for the new launch modes MicrosoftDocs/terminal#158

Merged

Add command line flag to start in focus mode #7875

Closed

7124: add comman line option for launching in focus mode

45c466d

Don-Vito marked this pull request as draft October 10, 2020 09:49

7124: add UT for multiple commands

73a4fe1

Don-Vito requested a review from Mike Griese (zadjii-msft) October 10, 2020 12:49

Don-Vito marked this pull request as ready for review October 10, 2020 12:49

Mike Griese (zadjii-msft) self-assigned this Oct 12, 2020

Mike Griese (zadjii-msft) approved these changes Oct 12, 2020

View reviewed changes

Mike Griese (zadjii-msft) removed their assignment Oct 12, 2020

ghost requested review from Dustin L. Howett (DHowett), PankajBhojwani and Leon Liang (leonMSFT) October 12, 2020 22:34

Dustin L. Howett (DHowett) requested changes Oct 13, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 13, 2020

Dustin L. Howett (DHowett) added the hacktoberfest-accepted label Oct 13, 2020

7124: add new launch modes to the schema in doc cascadia

cf9016c

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 13, 2020

Don-Vito requested a review from Dustin L. Howett (DHowett) October 13, 2020 06:43

7124: add new launch modes to the schema in doc cascadia

ad03ae0

Leonard Hecker (lhecker) reviewed Oct 13, 2020

View reviewed changes

Mike Griese (zadjii-msft) assigned Dustin L. Howett (DHowett) Oct 13, 2020

Don-Vito added 3 commits October 13, 2020 23:36

7124: remove explicit namespace and add comment the flags in AppComma…

97abb96

…ndLineArgs

7124: fix typo in a comment

1fb5e83

7124: fix another typo in a comment

bf1f23b

Dustin L. Howett (DHowett) approved these changes Oct 14, 2020

View reviewed changes

ghost added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Oct 14, 2020

Dustin L. Howett (DHowett) added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 14, 2020

ghost merged commit f86045e into microsoft:master Oct 14, 2020

This was referenced Oct 30, 2020

Apply Enum Mappings to most enum settings #8090

Closed

Add missing settings to Settings UI #8116

Merged

This was referenced Nov 11, 2020

focusMode option for launchMode setting #7825

Closed

Focus mode on app startup/launch #7124

Closed

This pull request was closed.

Conversation

Don-Vito commented Oct 9, 2020 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mike Griese (zadjii-msft) commented Oct 9, 2020

Uh oh!

Don-Vito commented Oct 9, 2020

Uh oh!

Mike Griese (zadjii-msft) commented Oct 9, 2020

Uh oh!

Don-Vito commented Oct 9, 2020

Uh oh!

Mike Griese (zadjii-msft) commented Oct 9, 2020

Uh oh!

Uh oh!

Don-Vito commented Oct 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mike Griese (zadjii-msft) left a comment

Choose a reason for hiding this comment

Uh oh!

Dustin L. Howett (DHowett) left a comment

Choose a reason for hiding this comment

Uh oh!

Don-Vito commented Oct 13, 2020

Uh oh!

Leonard Hecker (lhecker) left a comment

Choose a reason for hiding this comment

Uh oh!

Leonard Hecker (lhecker) Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

Don-Vito Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

Don-Vito commented Oct 13, 2020

Uh oh!

Don-Vito commented Oct 14, 2020

Uh oh!

Dustin L. Howett (DHowett) left a comment

Choose a reason for hiding this comment

Uh oh!

Dustin L. Howett (DHowett) commented Oct 14, 2020

I have several questions regarding the feature and the PR

Uh oh!

Deleted user (ghost) commented Oct 14, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Deleted user (ghost) commented Nov 11, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Don-Vito commented Oct 9, 2020 •

edited by DHowett

Loading

Don-Vito commented Oct 10, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.